Feature/add wincon gui#24
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds HUD score display and win detection: new Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant Controller
participant GameState
participant UI
Player->>Controller: performMove(destination)
Controller->>GameState: recordArrival(player, destination)
Controller->>GameState: incrementScore(player)
Controller->>Controller: doesPlayerWin()
alt winnerFound
Controller->>UI: update winConLabel (announce winner)
Controller->>UI: update currentPointLabel
Controller->>GameState: set wonGame flag
else noWinner
Controller->>UI: update currentPointLabel
Controller->>GameState: continue turn flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/TravelGameController.java (1)
353-360: Missing score increment indoContinueJourneywhen player arrives.The score is correctly incremented here in
doMovewhen a player arrives at their destination. However,doContinueJourney(lines 445-446) also handles arrival but does not callmanaged.increaseScore()ordoesPlayerWin(). A player completing their journey via continuation will not receive points and the win condition won't be checked.Proposed fix for doContinueJourney
if (managed.isTravelling()) { logList.getItems().add( "➡ " + safeName(managed) + " fortsätter mot " + targetName + " (rolled=" + journey.getDistanceMoved() + ", remaining=" + journey.getRemainingDistance() + ")" ); } else { logList.getItems().add("✅ " + safeName(managed) + " kom fram till " + targetName + "!"); + managed.increaseScore(); } tx.commit(); players.set(currentPlayerIndex, managed); - if (!managed.isTravelling() && !wonGame) { + if (!managed.isTravelling()) { + doesPlayerWin(); + } + if (!managed.isTravelling() && !wonGame) { currentPlayerIndex = (currentPlayerIndex + 1) % players.size(); }
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/TravelGameController.java`:
- Around line 378-386: In doesPlayerWin(), stop the loop after the first winner
is found to avoid overwriting winConLabel and multiple state changes: after
setting wonGame = true and updating winConLabel with player.getPlayerName(),
break out of the for loop (or return from the method) so only the first matching
Traveler (from players) wins; also remove the System.out.println debug call and
replace it with a proper logger call (or delete it) to use production logging
instead of stdout.
🧹 Nitpick comments (2)
src/main/java/org/example/TravelGameController.java (1)
28-28: Consider using aLabelinstead ofDialogPanefor the win message.
DialogPaneis typically used as the content root inside aDialogorAlert, not as a standalone element in a layout. While this may work functionally, aLabelwould be more idiomatic for displaying a simple win message. You could also hide it initially and make it visible when a winner is declared.src/main/resources/travelGame.fxml (1)
82-82: Consider visibility management for the win dialog.The
DialogPaneis always present in the layout. Since it starts empty, users may see an empty styled box. Consider either:
- Setting
visible="false"initially and toggling it in the controller when a winner is found- Using a
Labelwithvisible="false"as a simpler alternative- <DialogPane fx:id="winConLabel" style="-fx-font-size: 20px; -fx-font-weight: bold;"/> + <Label fx:id="winConLabel" visible="false" style="-fx-font-size: 20px; -fx-font-weight: bold; -fx-text-fill: gold;"/>Then in the controller:
winConLabel.setText(player.getPlayerName() + " Wins the game. Congratulations"); winConLabel.setVisible(true);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/TravelGameController.javasrc/main/resources/travelGame.fxml
🔇 Additional comments (2)
src/main/java/org/example/TravelGameController.java (1)
48-48: LGTM - Score display integration.The
currentPointLabelbinding and HUD update logic correctly displays the current player's score.Also applies to: 264-264
src/main/resources/travelGame.fxml (1)
69-70: LGTM - Points label addition.The "Points:" label and
currentPointLabelfollow the same pattern as the other HUD elements (Credits, Turn) and are correctly bound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private void doesPlayerWin() { | ||
| for (Traveler player : players) { | ||
| if (player.checkScore()) { | ||
| wonGame = true; | ||
| System.out.println(player.getPlayerName() + " Wins the game. Congratulations"); | ||
| winConLabel.setContentText(player.getPlayerName() + " Wins the game. Congratulations"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Loop continues after finding a winner; includes debug statement.
Two issues:
- The loop doesn't
returnorbreakafter settingwonGame = true. If multiple players meet the win condition simultaneously,winConLabelwill be overwritten multiple times. System.out.printlnshould be removed or replaced with proper logging for production code.
Proposed fix
private void doesPlayerWin() {
for (Traveler player : players) {
if (player.checkScore()) {
wonGame = true;
- System.out.println(player.getPlayerName() + " Wins the game. Congratulations");
winConLabel.setContentText(player.getPlayerName() + " Wins the game. Congratulations");
+ return;
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void doesPlayerWin() { | |
| for (Traveler player : players) { | |
| if (player.checkScore()) { | |
| wonGame = true; | |
| System.out.println(player.getPlayerName() + " Wins the game. Congratulations"); | |
| winConLabel.setContentText(player.getPlayerName() + " Wins the game. Congratulations"); | |
| } | |
| } | |
| } | |
| private void doesPlayerWin() { | |
| for (Traveler player : players) { | |
| if (player.checkScore()) { | |
| wonGame = true; | |
| winConLabel.setContentText(player.getPlayerName() + " Wins the game. Congratulations"); | |
| return; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/TravelGameController.java` around lines 378 - 386,
In doesPlayerWin(), stop the loop after the first winner is found to avoid
overwriting winConLabel and multiple state changes: after setting wonGame = true
and updating winConLabel with player.getPlayerName(), break out of the for loop
(or return from the method) so only the first matching Traveler (from players)
wins; also remove the System.out.println debug call and replace it with a proper
logger call (or delete it) to use production logging instead of stdout.
# Conflicts: # src/main/java/org/example/TravelGameController.java
# Conflicts: # src/main/java/org/example/TravelGameController.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/TravelGameController.java`:
- Around line 570-574: doContinueJourney() currently calls doesPlayerWin()
before tx.commit(), unlike doMove() which calls doesPlayerWin() after
committing; move the doesPlayerWin() invocation in doContinueJourney() to after
tx.commit() and after the players list is refreshed/updated so the win check
uses committed state and up-to-date player entities (ensure wonGame/UI updates
only happen post-commit).
- Around line 521-529: Remove the dead private helper
distinctLocationsById(List<Location>) since it is unused; delete the method
declaration (private List<Location> distinctLocationsById(...)) from
TravelGameController and ensure there are no remaining references to it
elsewhere; no functional changes needed because updateGraphicsWithMoves()
already handles deduplication with .distinct().
♻️ Duplicate comments (1)
src/main/java/org/example/TravelGameController.java (1)
510-518: Loop continues after finding a winner; includes debug statement.The issues previously identified remain unaddressed:
- The loop doesn't
returnorbreakafter settingwonGame = true, so if multiple players meet the win condition,winConLabelwill be overwritten.System.out.printlnis debug code that should be removed for production.Proposed fix
private void doesPlayerWin() { for (Traveler player : players) { if (player.checkScore()) { wonGame = true; - System.out.println(player.getPlayerName() + " Wins the game. Congratulations"); winConLabel.setContentText(player.getPlayerName() + " Wins the game. Congratulations"); + return; } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/TravelGameController.javasrc/main/resources/travelGame.fxml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/travelGame.fxml
🔇 Additional comments (3)
src/main/java/org/example/TravelGameController.java (3)
11-11: LGTM!The new imports and FXML field bindings for
winConLabel(DialogPane) andcurrentPointLabel(Label) are correctly declared and align with the UI additions in the FXML file.Also applies to: 30-30, 49-49
352-352: LGTM!The score display follows the established pattern for HUD updates.
490-499: LGTM!The score increment and win check integration is correctly placed: score increases only on arrival, win check occurs after commit, and turn progression is appropriately guarded by
!wonGame.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private List<Location> distinctLocationsById(List<Location> locations) { | ||
| java.util.Map<Long, Location> byId = new java.util.LinkedHashMap<>(); | ||
| for (Location l : locations) { | ||
| if (l != null && l.getId() != null) { | ||
| byId.putIfAbsent(l.getId(), l); | ||
| } | ||
| } | ||
| return new java.util.ArrayList<>(byId.values()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all usages of distinctLocationsById in Java files
rg -n "distinctLocationsById" --type=javaRepository: ithsjava25/project-jpa-grupp-4-g
Length of output: 208
🏁 Script executed:
# Check the actual file content around lines 521-529 to verify the method definition
sed -n '515,535p' src/main/java/org/example/TravelGameController.java | cat -nRepository: ithsjava25/project-jpa-grupp-4-g
Length of output: 921
🏁 Script executed:
# Check updateGraphicsWithMoves method around line 434 to see if it uses .distinct()
sed -n '430,445p' src/main/java/org/example/TravelGameController.java | cat -nRepository: ithsjava25/project-jpa-grupp-4-g
Length of output: 621
Remove unused method distinctLocationsById().
This method is defined at lines 521-529 but is never called anywhere in the codebase. The similar deduplication logic in updateGraphicsWithMoves() uses the stream .distinct() method instead, making this dead code that should be removed.
🤖 Prompt for AI Agents
In `@src/main/java/org/example/TravelGameController.java` around lines 521 - 529,
Remove the dead private helper distinctLocationsById(List<Location>) since it is
unused; delete the method declaration (private List<Location>
distinctLocationsById(...)) from TravelGameController and ensure there are no
remaining references to it elsewhere; no functional changes needed because
updateGraphicsWithMoves() already handles deduplication with .distinct().
| managed.increaseScore(); | ||
| doesPlayerWin(); | ||
| } | ||
|
|
||
| tx.commit(); |
There was a problem hiding this comment.
Inconsistent placement of doesPlayerWin() relative to transaction commit.
In doContinueJourney(), doesPlayerWin() is called before tx.commit() (line 574), whereas in doMove() it's called after the commit (line 495). This inconsistency could lead to issues:
- If the transaction rolls back after
doesPlayerWin()setswonGame = trueand updates the UI, the game state becomes inconsistent. - The
playerslist isn't updated until line 575, sodoesPlayerWin()may check stale entity references.
Proposed fix: Move win check after commit for consistency
} else {
logList.getItems().add("✅ " + safeName(managed) + " kom fram till " + targetName + "!");
managed.increaseScore();
- doesPlayerWin();
}
tx.commit();
players.set(currentPlayerIndex, managed);
+
+ if (!managed.isTravelling()) {
+ doesPlayerWin();
+ }
if (!managed.isTravelling() && !wonGame) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| managed.increaseScore(); | |
| doesPlayerWin(); | |
| } | |
| tx.commit(); | |
| } else { | |
| logList.getItems().add("✅ " + safeName(managed) + " kom fram till " + targetName + "!"); | |
| managed.increaseScore(); | |
| } | |
| tx.commit(); | |
| players.set(currentPlayerIndex, managed); | |
| if (!managed.isTravelling()) { | |
| doesPlayerWin(); | |
| } | |
| if (!managed.isTravelling() && !wonGame) { |
🤖 Prompt for AI Agents
In `@src/main/java/org/example/TravelGameController.java` around lines 570 - 574,
doContinueJourney() currently calls doesPlayerWin() before tx.commit(), unlike
doMove() which calls doesPlayerWin() after committing; move the doesPlayerWin()
invocation in doContinueJourney() to after tx.commit() and after the players
list is refreshed/updated so the win check uses committed state and up-to-date
player entities (ensure wonGame/UI updates only happen post-commit).
1 participant
win con in gui game
Summary by CodeRabbit
New Features
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.